AsyncHttpConsumer: Do not stop the consumer when exiting self.handle already
#1334
Conversation
…` already Earlier `self.handle` always disconnected the HTTP client and stopped the consumer when exiting. This meant that the consumer couldn't receive any messages from the channel layer at all, making it impossible to send e.g. messages from workers to connected HTTP clients via long polling or server-sent events.
|
There were reasons (tm) for the addition of the Also, it's a bit unexpected that tests pass. I mean it's nice obviously, but I also suspect that coverage is ... not very good in this particular area. |
There was a problem hiding this comment.
Hi @matthiask. Slow follow-up, but I think you're on the right track here.
tl;dr We shouldn't be catching the exception. Rather we should let it bubble up to Daphne, which can then handle:
- putting together a sensible response for the client.
- Ensuring that the consumer gets shut down properly.
I need to work on the exact set of tests I want here, but we have a lead :)
Thanks for your input. (I'm not going to merge this but I'll leave it open whilst I'm working on it.)
| @@ -43,6 +43,9 @@ def __init__(self, *args, **kwargs): | |||
| await self.send( | |||
| {"type": "http.response.body", "body": body, "more_body": more_body} | |||
| ) | |||
| if not more_body: | |||
| await self.disconnect() | |||
| raise StopConsumer() | |||
There was a problem hiding this comment.
This should be Daphne's job. (Via http_disconnect)
There was a problem hiding this comment.
@carltongibson in this case, for checking if the body is fully sent/empty, is the expectation/correctness that await self.disconnect() is the right move? Or is the disconnect & StopConsumer the wrong flow?
I'm curious for how we signal up to the event loop manager (e.g. daphne, uvicorn) that we're ready for the disconnect to take plase. I can see why he considered doing the raise but unsure what the solution should be.
(At this time, due to lack of experience in the lower layer of this code, the disconnect sort of seems right, but yes, definitely happy to hear what should happen).
There was a problem hiding this comment.
Hi @jheld.
I'm curious for how we signal up to the event loop manager (e.g. daphne, uvicorn) that we're ready for the disconnect to take plase.
My thought was that Daphne already knows that the response has finished at this point:
I think :) -- it was a couple of months ago when I looked at this.
| finally: | ||
| await self.disconnect() | ||
| raise StopConsumer() | ||
| await self.handle(b"".join(self.body)) |
There was a problem hiding this comment.
So simplifying just to await self.handle(...) the intention is that no matter what's going on with message (additional bytes in it or not), we want/need to let the runner (daphne etc) deal with/control the rest of the flow, naturally.
@carltongibson so this specific change is correct/ready?
There was a problem hiding this comment.
Hi @jheld. Yes, I think so.
The reason I haven't pushed on here is that I think we need to make sure Daphne is able to correctly handle the case where we started the response and then error-ed after some time (thinking SSE or such). Currently it'll send an entire response, where we need it to just finish up the current one. (Q: what would uvicorn do in that case?)
Tests for all that.
Also I think we can document: "if your consumer is liable to raise an exception, you should probably add logic to handle that, otherwise you're falling back to default handling at the ASGI server level". (Or such)
(I think the other PRs here are essentially trying to build that logic into the generic consumers...)
Make sense?
Earlier
self.handlealways disconnected the HTTP client and stoppedthe consumer when exiting. This meant that the consumer couldn't receive
any messages from the channel layer at all, making it impossible to send
e.g. messages from workers to connected HTTP clients via long polling or
server-sent events.